Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Bitwise opcode spec (AND, OR, XOR) #50

Merged
merged 4 commits into from
Nov 25, 2021

Conversation

scroll-dev
Copy link
Contributor

No description provided.

@scroll-dev
Copy link
Contributor Author

@miha-stopar @ChihChengLiang We have addressed previous comments. PTAL

- stack_pointer + 1
- pc + 1
- gas + 3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some lookups are needed, like:

  • first operand must come from the first element in the stack
  • second operand must come from the second element in the stack
  • result is at the new top of the stack

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, updated the doc

1. stack underflow: when stack is empty
2. gas out: remaining gas is not enough
```python
class Stack():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why Stack code is needed here?

a8s = [1]
b8s = [4]
c8s = [0]
check_xor(a8s, b8s, c8s)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_xor(a8s, b8s, c8s)
check_and(a8s, b8s, c8s)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 17 to 19
a8s = [1]
b8s = [4]
c8s = [5]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(a8s) is 1, how come the test pass for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

- stack_pointer + 1
- pc + 1
- gas + 3

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see more descriptions of how the opcode is checked in the circuit so that the reviewers/auditors know better how to review the circuit code. For example,
"We have two inputs a and b and an output c, all are EVM words. We break the EVM word into 32 bytes, lookup an AND table of 1-byte inputs, and apply the lookup to the 32 chunks of each a, b, and c to see if a[i] | b[i] === c[i] holds for i in 0~31"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more descriptions

@ChihChengLiang ChihChengLiang linked an issue Oct 22, 2021 that may be closed by this pull request
Copy link
Contributor

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@icemelon
Copy link
Collaborator

@miha-stopar Could you approve running workflows as well? Thank you.

@miha-stopar
Copy link
Contributor

@miha-stopar Could you approve running workflows as well? Thank you.

Done.


`a`, `b`, and `c` are all EVM words. We break three EVM words into 32 bytes and
apply the lookup to the 32 chunks of `a`, `b`, and `c` to see if
`a[i] OP b[i] == c[i]` holds for `i = 0..31`, where `OP` belongs to
Copy link
Collaborator

@han0110 han0110 Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using rust's range style, it should be 32 or =31, right? Since there are some other cases are using 0..31, so I think we should avoid such ambiguity.

Suggested change
`a[i] OP b[i] == c[i]` holds for `i = 0..31`, where `OP` belongs to
`a[i] OP b[i] == c[i]` holds for `i = 0..32`, where `OP` belongs to

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

- `b` is at the second position of the stack
- `c`, the result, is at the new top of the stack
- Apply the lookup to 32 tuples of `a, b, c` chunks,
`(a[i], b[i], c[i]), i = 0..31`, with opcode corresponding table
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Suggested change
`(a[i], b[i], c[i]), i = 0..31`, with opcode corresponding table
`(a[i], b[i], c[i]), i = 0..32`, with opcode corresponding table

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Collaborator

@han0110 han0110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@han0110 han0110 merged commit 8bbfc38 into privacy-scaling-explorations:master Nov 25, 2021
@icemelon icemelon deleted the bitwise branch November 25, 2021 07:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sample binary opcode
6 participants